Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(esbuild): make it possible to set keepNames with TSX_ESBUILD_KEEP_NAMES #659

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nemikolh
Copy link

Hey! Thanks for the really cool project ✨

I've run into an issue with tsx where I'm using it with puppeteer and I get a crash because __names is not defined.

It essentially boils down to this code:

const result = await page.evaluate(() => {
  function func() {}
});

getting turned into this by esbuild under keepNames: true:

const result = await page.evaluate(() => {
  function func() {
  }
  __name(func, "func");
});

the issue is that __name is not defined in the page and so this errors.

See this playground

Given that I have no use for keepNames: true in my project, it would be nice if tsx allowed one to opt-out from keepNames: true via an environment variable.

Thus this PR. Let me know what you think! 😃

@privatenumber
Copy link
Owner

I've rejected this change before because I don't want to expose esbuild, especially if we swap it out in the future (which is becoming more likely with OXC developments). After thinking about it again, I realize I enabled keepNames because I also enabled minify to keep the cache small—so maybe we could turn these off if --no-cache is passed.

What might bother me is the difference in behavior between using cache and not using cache. I also considered creating a noop __name and injecting it globally, or using regex to replace __name inline, but that could get complicated if __name appears in user code. WDYT?

@nicomouss
Copy link

nicomouss commented Jan 10, 2025

Hello there, I am landing here as a user of tsx, which we find very useful to streamline our development experience.

Although we are now reaching the limitation regarding not being able to access the underlying esbuild configuration.

In our workflow, we are using tsx (to run the apps on our dev machines directly from our typescript codebase) alongside esbuild (used on its own to transpile/bundle our typescript code into js to be shipped to production), and Vitest (to execute unit tests against our typescript codebase; Vitest uses esbuild under the hood). This setup keeps things unified, because we use the same underlying transpiler/bundler (esbuild) at every stage of our development workflow.

Now the nice stuff about Vitest is that it can reuse any esbuild config:

/**
 * vitest.config.ts
 */

import { defineConfig } from "vitest/config";
import esbuildOptions from "./esbuild/esbuild.config"; // Our esbuild config in its standalone file

export default defineConfig({
    esbuild: {
        ...esbuildOptions, // re-using our esbuild config
        define: { // here we expand the esbuild config with some specific stuff
            __TEST__: "true"
        },
    },
    /* ... */
});

That way of doing is great to keep predictable transpilations between unit tests and production builds for example, as both share the same esbuild setup.

Ideally, it would be nice to be able to do the same thing with tsx. Especially if we want to "define" compile-time variables (see here: https://esbuild.github.io/api/#define, and previous vitest.config.ts example), it is not possible to inject those with tsx, as the underlying esbuild configuration is not accessible.

Note that in Vitest, some esbuild options are "reserved" and cannot be modified anyway. The same thing could be done internally in tsx regarding the "minify" option, which could be set in stone as "true".

Regarding your idea on swapping esbuild to OXC in the future, Vite (and thus Vitest) project is actually going the same way by building its own "Rolldown" bundler based on OXC (see https://rolldown.rs/about). Rolldown will provide "esbuild Feature Parity", so you might want to keep an eye on this project as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants